Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nit: Add ListResourceTemplates to ClientRequest / ServerResult in schema.ts #120

Merged
merged 3 commits into from
Dec 18, 2024
Merged

nit: Add ListResourceTemplates to ClientRequest / ServerResult in schema.ts #120

merged 3 commits into from
Dec 18, 2024

Conversation

gsabran
Copy link
Contributor

@gsabran gsabran commented Dec 17, 2024 •
edited
Loading

Based on my understanding of the specs, it seems that those values were forgotten in the ClientRequest / ServerResult type.

Motivation and Context

This change aligns schema.ts with the description of the specs found in https://spec.modelcontextprotocol.io/specification/server/resources/

How Has This Been Tested?

NA

Breaking Changes

Possibly breaking Typescript compilation for developers referencing ClientRequest / ServerResult. Still mostly a bug fix.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@gsabran gsabran changed the title nit: Add ListResourceTemplatesResult to ServerResult in schema.ts nit: Add ListResourceTemplates to ClientRequest / ServerResult in schema.ts Dec 17, 2024
jspahrsummers
jspahrsummers approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, great catch. Thanks!

jspahrsummers
jspahrsummers previously requested changes Dec 17, 2024
View reviewed changes
Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsabran Can you please just run npm run generate:json to update the JSON Schema? Then we can get this in. 🙏

@gsabran
Copy link
Contributor Author

gsabran commented Dec 17, 2024

Ah yes I'll do

@gsabran
Copy link
Contributor Author

gsabran commented Dec 17, 2024

Side question: do you think there's a need to clarify the results for SubscribeRequest and UnsubscribeRequest ? Maybe just as a comment that they map to EmptyResult (my understanding) ?

There is no SubscribeResult / UnsubscribeResult and I didn't the the response described in https://spec.modelcontextprotocol.io/specification/server/resources/#subscriptions either

@dsp-ant
Copy link
Member

dsp-ant commented Dec 18, 2024

Side question: do you think there's a need to clarify the results for SubscribeRequest and UnsubscribeRequest ? Maybe just as a comment that they map to EmptyResult (my understanding) ?

There is no SubscribeResult / UnsubscribeResult and I didn't the the response described in https://spec.modelcontextprotocol.io/specification/server/resources/#subscriptions either

This might also be an oversight on our end.

@dsp-ant dsp-ant enabled auto-merge December 18, 2024 10:06
@dsp-ant dsp-ant merged commit 8008e38 into modelcontextprotocol:main Dec 18, 2024
@gsabran gsabran deleted the patch-1 branch December 18, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsp-ant dsp-ant dsp-ant approved these changes

@jerome3o-anthropic jerome3o-anthropic jerome3o-anthropic approved these changes

@jspahrsummers jspahrsummers Awaiting requested review from jspahrsummers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gsabran @dsp-ant @jspahrsummers @jerome3o-anthropic